Assertion failure: !child->IsRelocated() (Child should be in its ordinal position), at /src/accessible/generic/DocAccessible.cpp:2144 #0 mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) /src/accessible/generic/DocAccessible.cpp:2144:9 #1 mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) /src/accessible/base/NotificationController.cpp:818:18 #2 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1886:12 #3 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1886:12 #4 nsRefreshDriver::FinishedWaitingForTransaction() /src/layout/base/nsRefreshDriver.cpp:2189:5 #5 mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /src/gfx/layers/client/ClientLayerManager.cpp:529:32 #6 mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /src/gfx/layers/ipc/CompositorBridgeChild.cpp:536:8 #7 mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1473:20 #8 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2119:25 #9 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2049:17 #10 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1895:5 #11 mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:1928:15 #12 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14 #13 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:524:10 #14 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21 #15 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10 #16 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3 #17 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27 #18 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30 #19 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22 #20 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8 #21 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21 #22 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22 #23 main /src/browser/app/nsBrowserApp.cpp:304:16 #24 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291 #25 _start (/home/user/workspace/browsers/m-c-1507023849-asan-debug/firefox+0x41eaf4)

Only reproduces with Stylo enabled. INFO: Last good revision: 637c37f4362c0ea01a72edaba9fa5db742ce64f9 INFO: First bad revision: 6406fed0a0473626bdfd4307e27034ccd2ddf7f9 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=637c37f4362c0ea01a72edaba9fa5db742ce64f9&tochange=6406fed0a0473626bdfd4307e27034ccd2ddf7f9

Eitan, could you please classify this bug, whether it's something new or a known problem?

This seems something probably more related to aria-owns itself than my patch. My patch just happens to change the order notifications happen, but that shouldn't impact correctness.

I'm not familiar enough with aria-owns or the accessible code, so probably other people can spend their time better than I on this bug. Let me know if I should really try to figure out how all the investigation.

I... cannot enable a11y at all on my local build because of bug 1406827... And this doesn't seem to be reproducible without a11y enabled.

I'll take this.

This issue is not specific to Stylo. What I think is happening: 1. A container has an aria-owned child. 2. Two elements are inserted into the container: (a) an element that does not get an accessible (eg. empty inline frame). i. The TreeWalker walks past the end of the ordinal children, because it can't get an accessible for the element, and gets the first relocated child. (b) an element that gets an accessible (eg. div) i. The accessible is inserted *after* the relocated accessible. 3. We are now in a state where there is an ordinal child placed after the relocated child. A bad bad state. 4. DoARIAOwnsRelocation is called again (eg. another id is added to aria-owns), and we run into the above assertion. I have a browser test that reproduced this issue, and demonstrates the wrong order of children we end up with.

I could add a flag to TreeWalker to not go to the aria owns phase, but I'm scared of touching that stuff!

Thanks for taking this Eitan!

Comment on attachment 8917614 [details] [diff] [review] Don't insert children ordinal past relocated children. r?surkov Review of attachment 8917614 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/DocAccessible.cpp @@ +1854,5 @@ > TreeWalker finder(container); > if (finder.Seek(node)) { > + Accessible* child = mWalker.Scope(node); > + // Don't want to slip into the relocated children > + if (child && !child->IsRelocated()) { Not sure I understand how it works, Scope() should never return a relocated child. Do I miss something?

What's next here?

(In reply to alexander :surkov from comment #10) > Comment on attachment 8917614 [details] [diff] [review] > Don't insert children ordinal past relocated children. r?surkov > > Review of attachment 8917614 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/generic/DocAccessible.cpp > @@ +1854,5 @@ > > TreeWalker finder(container); > > if (finder.Seek(node)) { > > + Accessible* child = mWalker.Scope(node); > > + // Don't want to slip into the relocated children > > + if (child && !child->IsRelocated()) { > > Not sure I understand how it works, Scope() should never return a relocated > child. Do I miss something? It does. TreeWalker::Scope calls TreeWalker::Next[1]. If the ordinal children are exhausted we walk the aria-owned children as well[2]. 1. https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/accessible/base/TreeWalker.cpp#86 2. https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/accessible/base/TreeWalker.cpp#153

That's definitely a TreeWalker bug, which shall be addressed in TreeWalker. A scoped tree walker should never return ARIA owned children, since the scope node has to be contained by the context's node. The easiest way (probably not the nicest one), is to extend TreeWalker::mFlags by eScoped value. Sounds ok?

status-firefox57=wontfix unless someone thinks this bug should block 57

(In reply to alexander :surkov from comment #13) > That's definitely a TreeWalker bug, which shall be addressed in TreeWalker. > A scoped tree walker should never return ARIA owned children, since the > scope node has to be contained by the context's node. The easiest way > (probably not the nicest one), is to extend TreeWalker::mFlags by eScoped > value. Sounds ok? Eitan, any updates on this ARIA assertion failure?

Here is a patch with what I think Alex suggested. Going to wait for a happy try, because I don't know the extent of this change. r?surkov

Comment on attachment 8935153 [details] [diff] [review] Don't traverse into relocated children in scoped TreeWalker. r?surkov Looking green

Eitan, after your fix lands in Nightly 59, should we uplift it to Beta 58?

Comment on attachment 8935153 [details] [diff] [review] Don't traverse into relocated children in scoped TreeWalker. r?surkov Review of attachment 8935153 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments fixed ::: accessible/base/TreeWalker.cpp @@ +111,5 @@ > > // If ARIA owned child. > Accessible* child = mDoc->GetAccessible(childNode); > if (child && child->IsRelocated()) { > + if (child->Parent() != mContext || mFlags & eScoped) { If we get here and the walker is scoped, then scope is rather wrong, right? I would have assertion here. @@ +147,5 @@ > if (mPhase == eAtEnd) { > return nullptr; > } > > if (mPhase == eAtDOM || mPhase == eAtARIAOwns) { it'd be nice to assert if eAtARIAOwns && eScoped @@ +232,5 @@ > } > > if (mPhase == eAtEnd) { > mARIAOwnsIdx = mDoc->ARIAOwnedCount(mContext); > + mPhase = mFlags & eScoped ? eAtStart : eAtARIAOwns; eAtDOM I believe, and also it doesn't make sense to update mARIAOwnsIdx if scoped @@ +237,3 @@ > } > > if (mPhase == eAtARIAOwns) { could you please assert here if we are scoped? ::: accessible/base/TreeWalker.h @@ +40,1 @@ > nah, scoped should be set when we scope the walker, by default it shouldn't be scoped

(In reply to Chris Peterson [:cpeterson] from comment #19) > Eitan, after your fix lands in Nightly 59, should we uplift it to Beta 58? this is not trivial change, I would be extra careful about uplifting it.

Can you please take another look at this? Thanks! https://treeherder.mozilla.org/#/jobs?repo=try&revision=4adb97c374ccb32ac275dc8f5b8fec7ea39fa4ae

(In reply to Chris Peterson [:cpeterson] from comment #19) > Eitan, after your fix lands in Nightly 59, should we uplift it to Beta 58? No. I don't think this bug merits an uplift. It's an assertion that results in reordered accessible children in an obscure case.

Comment on attachment 8935469 [details] [diff] [review] Don't traverse into relocated children in scoped TreeWalker. r?surkov Review of attachment 8935469 [details] [diff] [review]: ----------------------------------------------------------------- there's no assertion in Seek(). ::: accessible/base/TreeWalker.cpp @@ +20,5 @@ > // TreeWalker > //////////////////////////////////////////////////////////////////////////////// > > TreeWalker:: > + TreeWalker(Accessible* aContext, uint32_t aFlags) : I meant to set this flag inside TreeWalker::Scope(). It's probably ok to have in constructor too, but do we really have use case for that? @@ +157,5 @@ > + return child; > + } > + } else { > + MOZ_ASSERT(mPhase != eAtARIAOwns, > + "Don't walk relocated children in scoped mode"); it'd be better to not create conditional path for MOZ_ASSERT only, I would put it on top MOZ_ASSERT(mPhase != eARIAOwns || !(mFlags & eScoped), "");

(In reply to alexander :surkov from comment #24) > Comment on attachment 8935469 [details] [diff] [review] > Don't traverse into relocated children in scoped TreeWalker. r?surkov > > Review of attachment 8935469 [details] [diff] [review]: > ----------------------------------------------------------------- > > there's no assertion in Seek(). > > ::: accessible/base/TreeWalker.cpp > @@ +20,5 @@ > > // TreeWalker > > //////////////////////////////////////////////////////////////////////////////// > > > > TreeWalker:: > > + TreeWalker(Accessible* aContext, uint32_t aFlags) : > > I meant to set this flag inside TreeWalker::Scope(). It's probably ok to > have in constructor too, but do we really have use case for that? The flags right are now immutable (you set them at construction, and they stay the same). It would seem scary to alter the TreeWalker's behavior after calling Scope(). > > @@ +157,5 @@ > > + return child; > > + } > > + } else { > > + MOZ_ASSERT(mPhase != eAtARIAOwns, > > + "Don't walk relocated children in scoped mode"); > > it'd be better to not create conditional path for MOZ_ASSERT only, I would > put it on top > > MOZ_ASSERT(mPhase != eARIAOwns || !(mFlags & eScoped), ""); Good idea.

(In reply to Eitan Isaacson [:eeejay] from comment #25) > (In reply to alexander :surkov from comment #24) > > > TreeWalker:: > > > + TreeWalker(Accessible* aContext, uint32_t aFlags) : > > > > I meant to set this flag inside TreeWalker::Scope(). It's probably ok to > > have in constructor too, but do we really have use case for that? > > The flags right are now immutable (you set them at construction, and they > stay the same). It would seem scary to alter the TreeWalker's behavior after > calling Scope(). It's the whole idea of Scope to change TreeWalker's scope, in other words it makes TreeWalker to walk inside of a node, and never look outside it. That also means a scoped tree walker can never return ARIA-owns accessible. There's a flaw in our logic: we need to make Scope asserting if they tried to scope a tree walker to a context node. If you add that assertion in the patch, then we should be in good shape.

(In reply to alexander :surkov from comment #26) > (In reply to Eitan Isaacson [:eeejay] from comment #25) > > (In reply to alexander :surkov from comment #24) > > > > > TreeWalker:: > > > > + TreeWalker(Accessible* aContext, uint32_t aFlags) : > > > > > > I meant to set this flag inside TreeWalker::Scope(). It's probably ok to > > > have in constructor too, but do we really have use case for that? > > > > The flags right are now immutable (you set them at construction, and they > > stay the same). It would seem scary to alter the TreeWalker's behavior after > > calling Scope(). > > It's the whole idea of Scope to change TreeWalker's scope, in other words it > makes TreeWalker to walk inside of a node, and never look outside it. That > also means a scoped tree walker can never return ARIA-owns accessible. > Does this mean we should add the flag to constructors that assign mAnchorNode to a value different than mContext's content? > There's a flaw in our logic: we need to make Scope asserting if they tried > to scope a tree walker to a context node. If you add that assertion in the > patch, then we should be in good shape. MOZ_ASSERT((mContext->IsDoc() ? mDoc->DocumentNode()->GetRootElement() : mContext->GetContent()) != mAnchorNode)? I would think that would be a valid anchor node if the user expands the scope to the entire context. It would still not be possible to completely unscope the TreeWalker which is what bothers me. TreeWalker is not documented, so I am double checking all this with you. Sorry for the hand holding..

(In reply to Eitan Isaacson [:eeejay] from comment #27) > (In reply to alexander :surkov from comment #26) > > (In reply to Eitan Isaacson [:eeejay] from comment #25) > > > (In reply to alexander :surkov from comment #24) > > > > > > > TreeWalker:: > > > > > + TreeWalker(Accessible* aContext, uint32_t aFlags) : > > > > > > > > I meant to set this flag inside TreeWalker::Scope(). It's probably ok to > > > > have in constructor too, but do we really have use case for that? > > > > > > The flags right are now immutable (you set them at construction, and they > > > stay the same). It would seem scary to alter the TreeWalker's behavior after > > > calling Scope(). > > > > It's the whole idea of Scope to change TreeWalker's scope, in other words it > > makes TreeWalker to walk inside of a node, and never look outside it. That > > also means a scoped tree walker can never return ARIA-owns accessible. > > > > Does this mean we should add the flag to constructors that assign > mAnchorNode to a value different than mContext's content? Probably not, since neither of them is not scoped, however it'd be good to make some tweaks to make that explicit. TreeWalker(Accessible* aContext, nsIContent* aAnchorNode, uint32_t aFlags = eWalkCache) - it is used to walk starting from anchor. Btw it has only one caller (https://dxr.mozilla.org/mozilla-central/source/accessible/generic/HyperTextAccessible.cpp#286), I think we can remove aFlags parameter, and set it to eWalkContextTree implicitly. Btw, eWalkCache is a dead one too. TreeWalker(DocAccessible* aDocument, nsIContent* aAnchorNode) - context is null, there's no way to hit aria-owns. However I don't see this constructor is used. It appears we can safely remove it. We can deal with these in follow up if you prefer. > > There's a flaw in our logic: we need to make Scope asserting if they tried > > to scope a tree walker to a context node. If you add that assertion in the > > patch, then we should be in good shape. > > MOZ_ASSERT((mContext->IsDoc() ? mDoc->DocumentNode()->GetRootElement() : > mContext->GetContent()) != mAnchorNode)? yep, this should work, however a bulletproof assertion would be the tree traversal until the anchor is reached #ifdef DEBUG nsINode* parentNode = anchorNode; do { parentNode = parentNode->GetFlattenedParentNode(); } while (parentNode && parentNode = ContextNode()); MOZ_ASSERT(parentNode, "Wrong tree walker scope"); #endif > I would think that would be a valid anchor node if the user expands the > scope to the entire context. It would still not be possible to completely > unscope the TreeWalker which is what bothers me. I would provide Unscope method to address the concern, but perhaps it's not that important. After all TreeWalker is not universal class and has quite restricted applicability. We could add a comment that 'Scoping is univertible operation, and once tree walker is scoped it cannot be unscoped.' > TreeWalker is not documented, so I am double checking all this with you. > Sorry for the hand holding.. No worries, my pleasure to unwind this stuff with you. It's often the great commenting goes as you revisit the code. So if there are ideas how to improve it, then it's worth to do.

Final round. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f934e47bf323ef5714932f9e8234c77d748f46b

Eithan, could you add assertion into TreeWalker::Seek (comment #20) and address first part of comment #28 about setting eScoped flag by Scope() method? If you are not sure that's a good idea, then to file a followup pls?

Comment on attachment 8936597 [details] [diff] [review] Don't traverse into relocated children in scoped TreeWalker. r?surkov Review of attachment 8936597 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments fixed ::: accessible/base/TreeWalker.cpp @@ +75,5 @@ > Reset(); > > mAnchorNode = aAnchorNode; > > + mFlags |= eScoped; in previous patch, you had an assertion for aAnchorNode, it'd be nice to get it back. @@ +161,5 @@ > + return child; > + } > + } else { > + MOZ_ASSERT(mPhase != eAtARIAOwns, > + "Don't walk relocated children in scoped mode"); pls replace that on non-else version, the assertion shouldn't have any effect on release builds, something like MOZ_ASSERT(!(mFlags & eScoped) || mPhase != eAtARIAOwns, "");

(In reply to alexander :surkov from comment #32) > Comment on attachment 8936597 [details] [diff] [review] > Don't traverse into relocated children in scoped TreeWalker. r?surkov > > Review of attachment 8936597 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments fixed > > ::: accessible/base/TreeWalker.cpp > @@ +75,5 @@ > > Reset(); > > > > mAnchorNode = aAnchorNode; > > > > + mFlags |= eScoped; > > in previous patch, you had an assertion for aAnchorNode, it'd be nice to get > it back. This is uncovering another bug where we try to insert the root 'html' element when the entire document is replaced. I'll file a followup bug with this assert and a fix for that case. > > @@ +161,5 @@ > > + return child; > > + } > > + } else { > > + MOZ_ASSERT(mPhase != eAtARIAOwns, > > + "Don't walk relocated children in scoped mode"); > > pls replace that on non-else version, the assertion shouldn't have any > effect on release builds, something like > MOZ_ASSERT(!(mFlags & eScoped) || mPhase != eAtARIAOwns, ""); Done.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/10b290c38643 Don't traverse into relocated children in scoped TreeWalker. r=surkov